Conversation
tmikula-dev
requested review from
lsulak,
oto-macenauer-absa and
petr-pokorny-absa
as code owners
|
AquaSec has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSix new documentation files were added under Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ci_local.sh`:
- Around line 9-16: The shell invocation currently expands unquoted $(git
ls-files '*.py') which breaks on filenames with whitespace; change the two calls
that pass $(git ls-files '*.py') to Black and Pylint to use NUL-delimited file
lists instead (e.g. use git ls-files -z '*.py' | xargs -0 black --check or git
ls-files -z '*.py' | xargs -0 pylint --fail-under=9.5) or capture into a
NUL-aware array (mapfile -d '' FILES < <(git ls-files -z '*.py') && black
--check "${FILES[@]}"); update the occurrences referencing black and pylint so
filenames are handled safely.
🧹 Nitpick comments (1)
.github/agents/reviewer.agent.md (1)
21-21: Consider the maintenance implications of hardcoded implementation details.The reference to
ROUTE_HANDLERSinevent_gate_lambda.pyprovides clear guidance but will require updates if the implementation pattern changes. Consider adding a note that this specification should be reviewed when architectural changes occur.
miroslavpojer
left a comment
miroslavpojer
left a comment
There was a problem hiding this comment.
I am missing the copilot rules file for reviewer. He is very helpfull.
I have checked usage of rulesets.
- I see there is some unsupported setup.
- When signed commit will be enabled then copilot refuse to work.
- It can be fixed by bypassing the copilot for PR only.
Could you introduce rulesets, add bypass and update the related handbook pages with this technical tip?
.github/copilot-instructions.md
Outdated
| - Handlers: `HandlerApi`, `HandlerToken`, `HandlerTopic`, `HandlerHealth` in `src/handlers/` | ||
| - Writers: `WriterEventBridge`, `WriterKafka`, `WriterPostgres` in `src/writers/` (inherit from `Writer` base class) | ||
| - Config: `conf/config.json`, `conf/access.json`, `conf/topic_schemas/*.json` | ||
| - Routes defined in `ROUTE_HANDLERS` dict in `event_gate_lambda.py` |
There was a problem hiding this comment.
maybe
- Terraform scripts are not part of this repository
There was a problem hiding this comment.
I reworked my proposed solution of copilot-instructions.md based on my current experience with agent development. I added your suggestion, please see the new version in 674cd11
There was a problem hiding this comment.
During the time there were several talks about experiences. See latest version in Release Notes Generate, There is last version mixing yours and my style.
.github/copilot-instructions.md
Outdated
| - Routes defined in `ROUTE_HANDLERS` dict in `event_gate_lambda.py` | ||
|
|
||
| Coding guidelines | ||
| - Keep changes small and focused |
There was a problem hiding this comment.
I'd add
- Preserve existing formatting and conventions.
ci_local.sh
Outdated
| @@ -0,0 +1,29 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Would be nice to add also a lint prompt to use this script.
Something like .github/prompts/lint.prompt.md with this content:
---
mode: agent
description: Lint code in the project using Pylint, Black, and mypy tools
---
Your task is to lint the code in the project using Pylint, Black, and mypy tools. Follow the instructions below to perform the linting process.
## Instructions
- run ci_local.sh - The command is going to take a while to finish, make sure you give it enough time to complete before looking for results.
- Analyze the output from each tool and identify any issues or areas for improvement in the code.
- Provide a summary of the linting results, including any errors or warnings found by each toolThere was a problem hiding this comment.
Based on my experience, having these scripts is counterproductive. It worked much better for me just have inside the configuration file "Quality gates (run after changes, fix only if below threshold)" and commands. It runs them every time after agent work.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/aquasec_repo_scan.yml:
- Around line 28-38: The workflow currently only verifies a static file
("test_aquasec_scan.sarif") and uploads it (steps "Verify SARIF file exists" and
"Upload Scan Results to GitHub Security"), so restore the real AquaSec scanning
steps or make the purpose explicit: either (A) replace the check-and-upload of
test_aquasec_scan.sarif with the original authentication, API call, scanner
invocation, SARIF conversion, and output path used by the "AquaSec Full
Repository Scan" job so actual scanning runs and generates a real SARIF file
before upload, or (B) if this is intended as a test, rename the workflow and job
and add a conditional (e.g., only: branches: [ci-test] or an env flag) and
documentation to clearly mark it as a mock; locate the steps by the step names
"Verify SARIF file exists" and "Upload Scan Results to GitHub Security" and the
test artifact name test_aquasec_scan.sarif to make the necessary edits.
This reverts commit 4b5662b.
Thanks for the comment! I personally do not find the copilot reviewer useful in comparison with CodeRabbit, which can be used in this OSS project. The decision is of course up to team. But myself as a main code contributor here, will not be using copilot reviewer. I am happy for discussion about the development workflow to understand yours better. I do an agent development on the other repository with same repo settings, and everything works correctly. |
miroslavpojer
left a comment
miroslavpojer
left a comment
There was a problem hiding this comment.
I see no use rulesets and no instruction file for copilot reviewer.
.github/copilot-instructions.md
Outdated
| - Handlers: `HandlerApi`, `HandlerToken`, `HandlerTopic`, `HandlerHealth` in `src/handlers/` | ||
| - Writers: `WriterEventBridge`, `WriterKafka`, `WriterPostgres` in `src/writers/` (inherit from `Writer` base class) | ||
| - Config: `conf/config.json`, `conf/access.json`, `conf/topic_schemas/*.json` | ||
| - Routes defined in `ROUTE_HANDLERS` dict in `event_gate_lambda.py` |
There was a problem hiding this comment.
During the time there were several talks about experiences. See latest version in Release Notes Generate, There is last version mixing yours and my style.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/copilot-instructions.md:
- Line 11: Update the sentence that currently reads "Terraform scripts are not
part of this repository" to clarify that production Terraform scripts are
excluded but example configurations exist; replace it with: "Production
Terraform scripts are not part of this repository; see terraform_examples/ for
reference configurations only." Ensure the phrasing appears in the same
paragraph/section where the original sentence is located so readers understand
the terraform_examples/ directory contains non-production examples.
🧹 Nitpick comments (1)
.github/copilot-instructions.md (1)
24-24: Consider clarifying the scope of the lazy initialization pattern.The directive that
__init__methods "must not raise exceptions" is prescriptive. While lazy initialization is valuable for deferred resource connections (writers, clients), fail-fast validation for required parameters or invalid arguments in constructors is also a valid pattern. Consider softening to: "__init__should defer expensive operations (connections, I/O) to first use; validate required parameters immediately."📖 Proposed refinement
-- `__init__` methods must not raise exceptions; defer validation and connection to first use (lazy init) +- `__init__` should defer expensive operations (connections, file/network I/O) to first use; validate required parameters immediately
As I mentioned in the last respond. In our team, we do not use the copilot reviewer, prefer the CodeRabbit tool instead. So for us in the moment it would be only extra file to take care about. It is just not relevant to our situation and workflow at the moment, might add later. I also looked at your solution and went one section after another. I updated our current copilot instructions in the commit: d8a70ef. Lot of the information in your solution were redundant to our current state, but I added some of them as well. I would look at your and review it again, since it is a lot human-like readable not needed for the copilot (sentences with dots, structuring the areas, explaining the ideas of conditions too much, which is default etc.) |
3 participants
Overview
Adds detailed agent role definitions for DevOps Engineer, Reviewer, SDET, Senior Developer, and Specification Master in the
.github/agents/directory. Introducescopilot-instructions.mdfile to have copilot instruction setted for the project.Release Notes
Related
Closes #107
Summary by CodeRabbit
Note: Internal process docs only; no end-user functionality changes.